docker: Add script for selecting different PHP versions #32929

anomiex · 2023-09-07T17:54:57Z

Proposed changes:

jetpack docker select-php (or select-php inside the container) is now avalable to install and run a different version of PHP inside the container.

This is something of a proof of concept. See the addition to tools/docker/README.md for some important caveats.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

#32914

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Temporarily change

jetpack/tools/docker/docker-compose.yml

Line 37 in 2fe018f

image: automattic/jetpack-wordpress-dev:latest

to refer to ghcr.io/automattic/jetpack-wordpress-dev:pr-32929 and restart your docker env (jetpack docker stop && jetpack docker up -d).

github-actions · 2023-09-07T17:56:08Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

`jetpack docker select-php` (or `select-php` inside the container) is now avalable to install and run a different version of PHP inside the container.

ice9js

Thanks Brad! Personally I'm not a huge fans of scripts or tools that cause you to make a mess and don't clean up after themselves, so that's where I'm coming from with my comments. I'm hoping maybe some of these adjustments will help with that.

Let me know what you think.

ice9js · 2023-09-11T10:25:30Z

tools/docker/README.md

+
+* Running `jetpack docker down` or otherwise recreating the containers will reset to the default PHP version.
+* If you're wanting the new version of PHP to be used to serve web requests, you'll need to `jetpack docker stop && jetpack docker up -d` or the like.
+* You may need to update Composer packages from inside the container before you can successfully run `phpunit` or the like, in order to install a version compatible with the new version of PHP.


I have a couple of questions here:

Do we expect anything else other than PHPUnit or its dependencies to cause issues?

If so, it seems to me these are potentially compatibility issues that need addressing.

For phpunit itself, maybe it would be interesting to consider using PHAR releases instead of composer. That way we can also have multiple versions installed if need be.

Do we expect anything else other than PHPUnit or its dependencies to cause issues?

Some of changelogger's Symfony deps probably will too. At least some PHPCS sniffs may crash with particularly old PHP as well.

If so, it seems to me these are potentially compatibility issues that need addressing.

There's nothing we can do about upstream projects deciding to drop support for older versions of PHP more aggressively than we do.

For phpunit itself, maybe it would be interesting to consider using PHAR releases instead of composer. That way we can also have multiple versions installed if need be.

That would make it harder for everything outside of the docker container to run phpunit, as then they'd have to download the appropriate phar instead of letting Composer handle it.

We already have things set up so Composer can choose the correct version of phpunit to install based on the PHP version. The problem is that Composer has to be run with that PHP version to do that, and that composer.lock can only lock one version (and checking that into the repo is usually recommended).

ice9js · 2023-09-11T10:32:09Z

tools/cli/commands/docker.js

@@ -328,6 +328,8 @@ const buildExecCmd = argv => {
 		opts.push( 'bash' );
 	} else if ( cmd === 'db' ) {
 		opts.push( 'mysql', '--defaults-group-suffix=docker' );
+	} else if ( cmd === 'select-php' ) {


While switching the default version on the entire PHP container could be useful when some manual debugging is required, I'm thinking the most common use case for this will be to run unit tests.
Maybe we could also expand the jetpack docker phpunit command with a --php= option like so:

jetpack docker phpunit /some/path/to/some/tests --php=7.1

What I find particularly appealing about this is the given version would only affect the single command, so you don't have to worry about switching back to what you had before.

Unfortunately it's not that simple considering the need for re-running composer to get a compatible version of phpunit (and maybe other deps, like if we keep the use of johnkary/phpunit-speedtrap), and in some cases upgrading or downgrading composer to get a compatible version of that.

We could certainly try something that would only work for jetpack docker phpunit if we want. It'd still need a script like up to line 60 of the one in this PR, and we might avoid screwing up the monorepo vendor/ by installing PHPUnit and whatever other deps out-of-tree (with a composer.json making use of .config.platform.php to get the right versions) and running from there instead.

We could certainly try something that would only work for jetpack docker phpunit if we want.

Proof of concept for that is in #32979.

ice9js · 2023-09-11T10:33:40Z

tools/cli/commands/docker.js

+						'Select the version of PHP for use inside the container. See documentation for important notes!',
+					builder: yargCmd => {
+						yargCmd.positional( 'version', {
+							describe: 'The version to select, or "default".',


Hmm, can we maybe put a list of supported version numbers in there? I personally find little things like that quite helpful, even if it's just to check the expected format. It's a small list anyway.

Adding some basic validation on that would be cool too.

Not here without manually hard-coding them. It depends on which versions are available from https://deb.sury.org/.

I didn't bother with validation here since the exec'ed command itself does what would be the same validation.

anomiex · 2023-09-27T21:05:33Z

Since this probably breaks too much other stuff, I think it's unlikely we'll go ahead with this. #32979 happened for the more limited case of jetpack docker phpunit --php.

anomiex added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Proposal [Pri] Normal labels Sep 7, 2023

anomiex self-assigned this Sep 7, 2023

github-actions bot added [Tools] Development CLI The tools/cli to assist during JP development. Docker Docs labels Sep 7, 2023

anomiex mentioned this pull request Sep 7, 2023

Docker Feature Request: Easy ability to change php versions for unit tests #32914

Closed

Base automatically changed from fix/docker-nodejs-install to trunk September 8, 2023 13:16

docker: Add script for selecting different PHP versions

42eabd8

`jetpack docker select-php` (or `select-php` inside the container) is now avalable to install and run a different version of PHP inside the container.

anomiex force-pushed the add/docker-php-switcher branch from e7af9d5 to 42eabd8 Compare September 8, 2023 13:24

ice9js reviewed Sep 11, 2023

View reviewed changes

anomiex mentioned this pull request Sep 11, 2023

docker: Add a --php option to jetpack docker phpunit #32979

Merged

3 tasks

anomiex closed this Sep 27, 2023

anomiex deleted the add/docker-php-switcher branch September 27, 2023 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker: Add script for selecting different PHP versions #32929

docker: Add script for selecting different PHP versions #32929

anomiex commented Sep 7, 2023 •

edited

Loading

github-actions bot commented Sep 7, 2023

ice9js left a comment

ice9js Sep 11, 2023

anomiex Sep 11, 2023

ice9js Sep 11, 2023

anomiex Sep 11, 2023

anomiex Sep 11, 2023 •

edited

Loading

ice9js Sep 11, 2023

anomiex Sep 11, 2023

anomiex commented Sep 27, 2023

docker: Add script for selecting different PHP versions #32929

docker: Add script for selecting different PHP versions #32929

Conversation

anomiex commented Sep 7, 2023 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Sep 7, 2023

ice9js left a comment

Choose a reason for hiding this comment

ice9js Sep 11, 2023

Choose a reason for hiding this comment

anomiex Sep 11, 2023

Choose a reason for hiding this comment

ice9js Sep 11, 2023

Choose a reason for hiding this comment

anomiex Sep 11, 2023

Choose a reason for hiding this comment

anomiex Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

ice9js Sep 11, 2023

Choose a reason for hiding this comment

anomiex Sep 11, 2023

Choose a reason for hiding this comment

anomiex commented Sep 27, 2023

anomiex commented Sep 7, 2023 •

edited

Loading

anomiex Sep 11, 2023 •

edited

Loading